Skip to content
This repository has been archived by the owner on Dec 7, 2021. It is now read-only.

Two minor changes #55

Merged
merged 2 commits into from
Nov 20, 2017
Merged

Two minor changes #55

merged 2 commits into from
Nov 20, 2017

Conversation

asppsa
Copy link
Contributor

@asppsa asppsa commented Nov 20, 2017

  • Deleted an if-statement that wasn't needed (as the statement was nested inside an identical "if" at line 2278)
  • "Fixed" the AMD import. I think I did this correctly, but you may disagree. The define function is supposed to have as its second argument a function that will be called, passing in the module's dependencies. I guess another way to do this would be define([], function() { return factory(plugin); });, if it is necessary to pass that plugin variable in still.

@Mobius1 Mobius1 merged commit d25d1f8 into Mobius1:master Nov 20, 2017
@skeptic35
Copy link

@asppsa actually what you describe as "another way" would be the correct way.
The way it is now, the factory function gets called as soon as Selectr is loaded, meaning the Selectr constructor gets called without any arguments and throws the following error: "You must supply either a HTMLSelectElement or a CSS3 selector string." AMD loading is thus effectively broken.

Passing in the plugin parameter is imho completely unneccessary, as it is never used in the factory function. Also the first parameter to define can be ommitted, as there are no dependencies being passed in.
So to sum it up, the code should look like this:

    if (typeof define === "function" && define.amd) {
        define(function(){return factory()});

@asppsa
Copy link
Contributor Author

asppsa commented Dec 11, 2017

@skeptic35, are you saying that the fix in 73eed26 generates the error you describe? It seems to be working for me, although I'm JSPM rather than RequireJS.

@skeptic35
Copy link

@asppsa yup - that's what I'm saying. Don't know why it is working for you because it actually doesn't have anything to do with RequireJS, it's just how javascript works.
This:

define([], factory(plugin));

calls the factory function and passes it's return to the define, while this

define([], factory);

doesn't call the function and instead passes the function itself to define, which is exactly what we want here.

I have to apologize though for the suggested solution. Was in a bit of a hurry, so I didn't really think this through.
As there are absolutely no dependencies being passed in there is also abosolutely no need for the function returning the factory. Meaning this here would be much more easy and would also work:

define(factory);

In short: Just make sure the factory function doesn't get called and we're fine ;)

@skeptic35
Copy link

P.S. - come to think of it - maybe it does have to do with using RequireJS. Dunno how JSPM works, but RequireJS calls the function passed to define as soon as the according file is required.

@asppsa
Copy link
Contributor Author

asppsa commented Dec 11, 2017

@skeptic35, my PR changed line 11 from define([], factory(plugin)); to define([], factory);. Isn't this what you are saying should work?

@skeptic35
Copy link

Oops - you're right - it did. That's kind of embarassing - sorry.
See, the thing is that I installed Selectr just yesterday via npm. I saw it throw that error, saw the define([], factory(plugin) in the code, saw your PR and very obviously drew totally wrong conclusions without really checking which way round your PR was.
Please accept my apologies.
Now I just have to find out why the version I installed yesterday says it's a 2.4.0 (same as the current version) but still contains the bug you fixed...

@asppsa
Copy link
Contributor Author

asppsa commented Dec 12, 2017

No need to apologise! I was definitely not sure about my patch, so I am glad someone using requirejs can test things out.

I think the issue is that there has not been a new release since the PR was merged. You could perhaps try installing from git instead of and see if that works.

@skeptic35
Copy link

You could perhaps try installing from git instead of and see if that works.

I kinda did that already ;) - works

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants